Skip to content

Conversation

mochaaP
Copy link
Contributor

@mochaaP mochaaP commented Sep 10, 2025

  • Add meson build system
  • Sync cmake's installation layout to Makefile and Meson builds

@mochaaP mochaaP changed the title master Add meson build system Sep 10, 2025
CMakeLists.txt Outdated
)
install (FILES pystring.h
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean it will no longer install in a pystring subdirectory? So software that had been doing

#include <pystring/pystring.h>

will stop working?

Copy link
Contributor Author

@mochaaP mochaaP Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do this the other way around? Since we don't export a config file (.cmake/.pc) people either vendor it or do find_path(PYSTRING_INCLUDE_DIR pystring.h)

pystring.h: 25 repos
image

pystring/pystring.h: 7 repos
image

vcpkg and conan.io actively patch out all <pystring/pystring.h> to <pystring.h> or in reverse:
image

Copy link
Collaborator

@grdanny grdanny Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the conversions in:
#49
I think we do want to keep installing the header file to the /${PROJECT_NAME} sub directory.
That is the practice in different open source projects that we maintain, so wanting to keep it consistent.
Is there something stopping meson from doing that so that meson and cmake match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack. will adjust meson & makefile to that convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also should we keep -I<prefix>/include/pystring in pkg-config so #include <pystring.h> still works if one uses pkg-config?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about pkg-config. Are we supposed to provide a .pc file for that?

But with the adjustment to the /${PROJECT_NAME} sub directory this looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done making it use the <pystring/pystring.h> form.

@lgritz
Copy link
Member

lgritz commented Sep 10, 2025

I would strongly recommend that this only be accepted if it also contains modifications that add cases to the CI test matrix that build pystring with meson and also run the tests.

Without an automated test to ensure that the meson build always works, I think it's a pile of divergence-caused build bugs just waiting to happen.

@lgritz
Copy link
Member

lgritz commented Sep 10, 2025

(If I'm being honest, I would also recommend that the .cpp be eliminated and make the entire pystring be a single file header-only library, and then you don't really need much of a "build system" at all.)

@mochaaP
Copy link
Contributor Author

mochaaP commented Sep 11, 2025

I would strongly recommend that this only be accepted if it also contains modifications that add cases to the CI test matrix that build pystring with meson and also run the tests.

sure, working on ci.

@mochaaP mochaaP force-pushed the master branch 5 times, most recently from 9828288 to ede9e51 Compare September 12, 2025 17:04
Copy link
Collaborator

@grdanny grdanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. let's run with this and see how it goes.

@grdanny grdanny enabled auto-merge October 6, 2025 04:19
@grdanny grdanny disabled auto-merge October 6, 2025 04:21
@grdanny grdanny enabled auto-merge October 6, 2025 04:21
@grdanny grdanny disabled auto-merge October 6, 2025 04:30
@grdanny grdanny merged commit a09708a into imageworks:master Oct 6, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants